lib: emit events about connection messages #252

lundibundi · 2017-07-11T14:02:22Z

As discussed I add events to the connection, also events about connection establishment and destruction already exist on server ('connect', 'disconnect').
Also I did not forward connection events through a server as this will most likely have a significant performance cost, especially during highload. And it's easy to implement if user ever needs it:

server.on('connect', (connection) => {
  common.forwardMultipleEvents(connection, server,
    ['handshakeRequest', 'call', 'event', 'inspect']);
});

Fixes: #250

belochub · 2017-07-11T18:36:16Z

lib/connection.js

@@ -384,6 +384,8 @@ class Connection extends EventEmitter {

    this.server.startSession(this, application, authStrategy, credentials,
      this._onSessionCreated.bind(this));
+
+    this.emit('handshakeRequest', applicationName, authStrategy, credentials);


I don't think we should emit credentials here.

@belochub also not sure about that, but I thought that users may decide for themselves whether they need to log them or just ignore this parameter. @metarhia/jstp-core?

@lundibundi, it won't be secure that way.

@belochub It won't be secure it user decides to log them, but they may have different trade-offs there.

@lundibundi, I meant providing the user an ability to log them isn't secure.

@lundibundi, ping. You haven't changed it.

Yes because I see no need to, we are not responsible for users breaking their security by themselves.

belochub · 2017-07-11T18:48:02Z

@lundibundi, we also discussed the possibility to log all of the incoming/outgoing messages in debug mode, and I haven't found the implementation of corresponding events here. Will you add it later?

lundibundi · 2017-07-12T09:50:28Z

@belochub yeah, but should we just emit them as well when node is in debug mode?

nechaido

The functionality is ok, but i don't like the tests. If you add new functionality, you should provide new tests, that will pass with suggested changes but won't pass without them. Existing tests must be changed only after breaking API change.

nechaido · 2017-07-12T09:58:26Z

test/node/connection-call.js

@@ -13,6 +13,17 @@ const serverConfig =
 let server;
 let connection;

+function addCallEventCheck(test, connection, iface, calledName, providedArgs) {


I'd prefer to add one test that will check that event is emited with the right payload, there is no need to test this every time. If you insist on doing so, I'd prefer the function to be at the end of file.

belochub · 2017-07-12T10:13:37Z

@lundibundi, we discussed that there will be different modes and in "debug" or "verbose" mode all of the messages will be emitted.

aqrln · 2017-07-13T20:55:44Z

test/node/connection-emit-actions.js

+
+const application = new jstp.Application(app.name, app.interfaces);
+const serverConfig =
+  { applications: [application], authPolicy: app.authCallback };


Style nit: maybe split this object over several lines? It's a bit long.

Sure. Btw we've got the same code in connection-call.js, We should probably also change it there.

aqrln · 2017-07-13T20:56:41Z

test/node/connection-emit-actions.js

+    const port = server.address().port;
+    const socket = net.connect(port);
+    socket.on('error',
+      () => test.fail('must create socket and connect to server'));


Nit: can you please use braces here?

aqrln · 2017-07-13T20:57:09Z

test/node/connection-emit-actions.js

+      () => test.fail('must create socket and connect to server'));
+    socket.on('connect', () => {
+      serverConnection.on('handshakeRequest',
+      (applicationName, authStrategy, credentials) => {


There's a missing indentation level. ~~It's strange that ESLint does not complain, it's exactly the bug that was fixed in the indent rule in eslint@4, AFAIU.~~ UPD: nope, it does. The CI has failed because of it.

Yeah, I forgot to update local eslint.

aqrln · 2017-07-13T20:59:33Z

test/node/connection-emit-actions.js

+    const port = server.address().port;
+    const socket = net.connect(port);
+    socket.on('error',
+      () => test.fail('must create socket and connect to server'));


Could you please add braces here too?

aqrln · 2017-07-13T20:59:44Z

test/node/connection-emit-actions.js

+      () => test.fail('must create socket and connect to server'));
+    socket.on('connect', () => {
+      serverConnection.on('handshakeRequest',
+      (applicationName, authStrategy, credentials) => {


Missing indentation.

aqrln

LGTM with style nits.

nechaido

LGTM

nechaido · 2017-07-18T12:28:18Z

test/node/connection-emit-actions.js

+        test.equal(connection.sessionId, app.sessionId,
+          'session id must be equal to the one provided by authCallback');
+        connection.close();
+        test.end();


Missed this bit, is it absolute that this callback will be called after both handshake event handlers? If not - test will fail because of assertions after end, in that case the right way to do it is using test.plan(). Same applies to every test here.

Yes, that's why I made them like this. Though that's implementation specific to be precise, so maybe it'll be better to use plan? @metarhia/jstp-core should we switch to plan here?

IMHO, if we multiple callbacks\handlers must be called, it is better to use test.plan().

Oh well, nobody seems to bother, but I guess the less implementation specific test are the better so I'll change it.

nechaido

Removed approve until #252 (review).

lundibundi · 2017-07-19T14:42:00Z

@metarhia/jstp-core ping.

nechaido

LGTM

belochub · 2017-07-20T11:24:18Z

lib/connection.js

@@ -377,6 +397,8 @@ class Connection extends EventEmitter {
      this, application, authStrategy, credentials,
      this._onSessionCreated.bind(this)
    );
+
+    this.emit('handshakeRequest', applicationName, authStrategy, credentials);


@lundibundi, I am still against emitting credentials here.

@belochub And I still see no harm in doing that. Let's wait what others have to say on this matter.

@belochub @lundibundi what if we emit them only in debug mode, but in release mode we omit them?

It makes no difference, in debug mode you have whole packets emitted, so there is no reason to make such a switch. Either we emit them or we don't, as I see it.

@lundibundi if you put it this way, I agree with @belochub because I see no use in emiting credential in release mode.

@lundibundi, that's why I'm opting for not emitting them here: if a user will need to debug application including authentication he/she can easily do that by getting complete messages.
And outside of the debug mode emitting credentials makes no sense, no one should use them in that case, because logging them will lead to security problems (and you agree on this).

@belochub Okay, I'll remove it soon.

nechaido · 2017-07-21T09:43:16Z

@tshemsedinov ping.

belochub · 2017-07-25T17:15:01Z

test/node/connection-emit-actions.js

+    addCallEventCheck(test, server.getClients()[0], iface, methodName, args);
+    connection.on('callback', (error, ok) => {
+      test.assertNot(error, 'callMethod must not return an error');
+      test.strictSame(ok, [42], 'Ok contents must match');


belochub · 2017-07-25T17:21:53Z

test/node/connection-emit-actions.js

+  }
+);
+
+test.test('must emit event on call with no arguments and return value',


"with no arguments and return value" is a bit misleading: it is not clear if "no" does also relate to "return value".

Also we should change this in the connection tests, as it's the same there.

belochub · 2017-07-25T17:23:15Z

test/node/connection-emit-actions.js

+  });
+});
+
+test.test('must emit packets in not "production" mode', (test) => {


It's either "development" or "production" mode, you can replace "not production" with "development".

nechaido · 2017-07-25T17:36:23Z

test/node/connection-emit-actions.js

+      test.fail('must create socket and connect to server');
+    });
+    socket.on('connect', () => {
+      serverConnection.on('handshakeRequest',


What is the reason of adding listeners here but not on line 53?

I just wanted to put them in one place, also this callback should always be called after the one above, but it's okay to put them there too.

nechaido · 2017-07-25T17:45:41Z

test/node/connection-emit-actions.js

+      test.fail('must create socket and connect to server');
+    });
+    socket.on('connect', () => {
+      serverConnection.on('handshakeRequest',


IMHO this test is also redundant.

I'd test both as I see no harm in doing so.

nechaido · 2017-07-25T17:49:19Z

test/node/connection-emit-actions.js

+  }
+);
+
+test.test('must emit event on call with no arguments and return value',


This tests is redundant. Bacause of this addCallEventCheck() should be inlined in a previous test.

nechaido · 2017-07-25T18:01:02Z

test/node/connection-emit-actions.js

+  // 4 packets from call below and 4 from 1 heartbeat
+  test.plan(8);
+
+  const clientSentPackets = [{}, { call: [1, 'calculator'], answer: [] }];


I'd prefer this test not to start handshake. this way there will be no need for tap-oneOf.

@nechaido You meant heartbeat? I don't really care, I thought that we should test that it emits heartbeat packets as it's easy to miss them especially with our current implementation that writes them directly to transport.

@lundibundi I mean that we should test them separately.

nechaido · 2017-07-25T18:05:11Z

lib/connection.js

@@ -193,6 +197,10 @@ class Connection extends EventEmitter {
  _heartbeatCallback(interval) {
    this.transport.send('{}');
    this.setTimeout(interval, this._heartbeatCallbackInstance);
+
+    if (process.env.NODE_ENV !== 'production') {
+      this.emit('sentPacket', this._heartbeatMessage);


Heartbeat should have it's own heartbeat event.

coveralls · 2017-07-26T09:27:31Z

Coverage increased (+1.2%) to 82.192% when pulling 900917b on add-conn-events into 4d76172 on master.

tshemsedinov

I propose to rename events:
sentMessage -> outgoingMessage
receivedMessage -> incomingMessage

nechaido

LGTM

belochub · 2017-07-29T13:57:19Z

test/node/connection-emit-actions.js

+  test.plan(2);
+
+  server.getClients()[0].on('heartbeat', (message) => {
+    test.strictSame(message, {}, 'Heartbeat message must match on server side');


You should decapitalize "Heartbeat" here.

belochub · 2017-07-29T13:57:27Z

test/node/connection-emit-actions.js

+    test.strictSame(message, {}, 'Heartbeat message must match on server side');
+  });
+  connection.on('heartbeat', (message) => {
+    test.strictSame(message, {}, 'Heartbeat message must match on client side');


Here as well.

tshemsedinov

LGTM after message renaming

belochub

LGTM

Fixes: #250 PR-URL: #252 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Timur Shemsedinov <timur.shemsedinov@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub · 2017-07-29T22:41:22Z

Landed in 04ed72e.

Fixes: #250 PR-URL: #252 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Dmytro Nechai <nechaido@gmail.com> Reviewed-By: Timur Shemsedinov <timur.shemsedinov@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) * **parser:** fix a possible memory leak *(Alexey Orlenko)* [#44](#44) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** make parser single-pass *(Mykola Bilochub)* [#61](#61) * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **parser:** fix bug causing node to crash *(Mykola Bilochub)* [#75](#75) * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **test:** add JSON5 specs test suite *(Alexey Orlenko)* [#158](#158) * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **parser:** implement NaN and Infinity parsing *(Mykola Bilochub)* [#201](#201) * **parser:** improve string parsing performance *(Mykola Bilochub)* [#220](#220) * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **lib:** implement API versioning *(Denys Otrishko)* [#231](#231) **\[semver-minor\]** * **lib:** allow to set event handlers in application *(Denys Otrishko)* [#286](#286) **\[semver-minor\]** * **lib:** allow to broadcast events from server *(Denys Otrishko)* [#287](#287) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

lundibundi added connection lib semver-minor server labels Jul 11, 2017

lundibundi self-assigned this Jul 11, 2017

lundibundi requested review from belochub, tshemsedinov, aqrln and nechaido July 11, 2017 14:02

belochub reviewed Jul 11, 2017

View reviewed changes

nechaido requested changes Jul 12, 2017

View reviewed changes

aqrln reviewed Jul 13, 2017

View reviewed changes

aqrln approved these changes Jul 13, 2017

View reviewed changes

nechaido approved these changes Jul 18, 2017

View reviewed changes

nechaido reviewed Jul 18, 2017

View reviewed changes

nechaido self-requested a review July 18, 2017 12:32

lundibundi force-pushed the add-conn-events branch from ad6d3cf to 2438231 Compare July 19, 2017 14:40

nechaido approved these changes Jul 20, 2017

View reviewed changes

belochub reviewed Jul 20, 2017

View reviewed changes

nechaido added this to the 1.0.0 milestone Jul 21, 2017

belochub reviewed Jul 25, 2017

View reviewed changes

nechaido requested changes Jul 25, 2017

View reviewed changes

lundibundi force-pushed the add-conn-events branch from bee03c5 to 900917b Compare July 26, 2017 09:19

metarhia deleted a comment from coveralls Jul 27, 2017

lundibundi added 4 commits July 28, 2017 11:46

lib: emit events about connection messages

9539fe9

fixup! lib: emit events about connection messages

b2f1ecb

fixup! apply packet -> message renaming in tests

3234c17

fixup! revert package-lock to master

4a255a7

lundibundi force-pushed the add-conn-events branch from 900917b to 4a255a7 Compare July 28, 2017 09:00

tshemsedinov reviewed Jul 29, 2017

View reviewed changes

nechaido approved these changes Jul 29, 2017

View reviewed changes

belochub reviewed Jul 29, 2017

View reviewed changes

tshemsedinov approved these changes Jul 29, 2017

View reviewed changes

fixup! rename packet messages, fix typos

f107593

belochub approved these changes Jul 29, 2017

View reviewed changes

belochub closed this Jul 29, 2017

belochub deleted the add-conn-events branch July 29, 2017 22:41

belochub mentioned this pull request Jul 29, 2017

lib: support different loggers #224

Closed

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

lib: emit events about connection messages #252

lib: emit events about connection messages #252

Conversation

lundibundi commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub commented Jul 11, 2017

lundibundi commented Jul 12, 2017

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln left a comment

Choose a reason for hiding this comment

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido left a comment • edited Loading

Choose a reason for hiding this comment

lundibundi commented Jul 19, 2017

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

lundibundi Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 26, 2017 • edited Loading

tshemsedinov left a comment

Choose a reason for hiding this comment

nechaido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshemsedinov left a comment

Choose a reason for hiding this comment

belochub left a comment

Choose a reason for hiding this comment

belochub commented Jul 29, 2017

aqrln Jul 13, 2017 •

edited

Loading

nechaido left a comment •

edited

Loading

nechaido Jul 20, 2017 •

edited

Loading

lundibundi Jul 20, 2017 •

edited

Loading

belochub Jul 20, 2017 •

edited

Loading

nechaido Jul 25, 2017 •

edited

Loading

lundibundi Jul 26, 2017 •

edited

Loading

coveralls commented Jul 26, 2017 •

edited

Loading